Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create DAO and New Endpoints for Program Mentorship Relationship #22

Open
wants to merge 4 commits into
base: ms-backend-server
Choose a base branch
from

Conversation

decon-harsh
Copy link

Description

This is a cross project issue related to Bridge In Tech, For creating a Program-Mentor-Mentee Relation there was a need to create a new DAO and some endpoints to create and accept those relations.

Fixes #19

Type of Change:

  • Code

Code/Quality Assurance Only

  • This change requires a documentation update (software upgrade on readme file)
  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
  • Update Swagger documentation and the exported file at /docs folder

Code/Quality Assurance Only

  • My changes generate no new warnings
  • My PR currently breaks something (fix or feature that would cause existing functionality to not work as expected)
  • New and existing unit tests pass locally with my changes

@decon-harsh
Copy link
Author

@mtreacy002 Please have a look at the logic and codes. This is ongoing and I will make the changes in the text/swagger docs later? Will that work? Actually, I wanted to seek your green signal on this.

I have checked

  • All the possible program mentorship relations are created
  • And accepted for now.

This is a little bit different from the gists.
I filled mentorship_relation.accept_date to datetime().now.timestamp() when request is being accepted for the first time.It causes no problem Moreover it even helped me to check the state of relation.

However, we may have one small problem and that is this case:

Both the time when a program sends successful second request to a mentor or mentee. The database becomes something like

 Mentor_id = Mentor_id
 Mentee_id = Mentee_id
 Action_user = org_rep_id 

The problem arises while accepting this relation, as (I think )this is the only common case, both mentor or mentee can accept this relation.

But, this problem can be solved easily in BIT when we check the relation_extension_model.mentee/mentor_accept_date.

Can you suggest me a better way to solve it in MS only.

@decon-harsh
Copy link
Author

@mtreacy002 Also the email templates are not appropriate I will soon create new one too!

@decon-harsh
Copy link
Author

I created only one endpoint for sending both the type of request to anyone regardless of their role.

Why I did this?

This surely made the logic a bit tough. But on the other hand in BIT it will be easy to use a single endpoint. Just by changing the datas one can easily create the request . Moreover I tried to keep MS logics and validations at MS side and BIT's logics and validations seperate.

Let me know what do you think of this?

Copy link
Owner

@mtreacy002 mtreacy002 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@decon-harsh . Good job submitting this code changes considering this is a complex issue. The feedback below is just the my initial review.
Pointers:
Can you write with fewer lines of code per function? don't try to tackle all tasks in a single function. I'd rather you use about 20 - 30 lines per function. For example the create_program_mentorship_relation function inside ProgramMentorshipRelationDAO has about 300 lines of code. This really makes It hard to read. Please divide these into sub functions. Make sure you check other places where you did the same.
So, please make changes to the things I point out here. After that I will check the logic.

required=True, description="Mentorship relation Organization Representative's ID"
),
"relation_id": fields.Integer(
description="Mentorship relation Organization Representative's ID"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this attribute the same with line 55 but just with different name? do we need this attribute?

@@ -137,6 +137,63 @@ def send_email_mentorship_relation_accepted(request_id):
)
send_email(request_sender.email, subject, html)

def send_email_program_mentorship_relation_accepted(request_id,org_rep_id):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we place this in a separate file "bit_email_utils.py" to make it easier for contributors to locate BIT specific email logic from.

MINIMUM_MENTORSHIP_DURATION
"""

def create_program_mentorship_relation(self, user_id: int, org_rep_id:int , mentor_id:int, mentee_id:int, relation_id:int, data: Dict[str, str]):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

org_rep_id:int , mentor_id:int, mentee_id:int,

can be removed and placed inside "data"

relation_id:int,

not needed

@decon-harsh
Copy link
Author

decon-harsh commented Mar 7, 2021

@mtreacy002 Please have a look! 😄 . I had left the minor changes in swagger docs or writing comments for later as I have my semester exams (Pretty rough week 😆 ).

But when I saw you requested changes in DAO I immediately started doing them, Talking about the DAO function I divided it into four smaller functions each with a specific task. I guess everything is working fine. The API endpoint can create requests. The other API endpoint can accept them.

I wanted to specifically ask you about request body of the endpoint. I made a single endpoint capable of creating all diff program mentorship relation but we have to give him different data. For example

A program sending an initial request to a mentor will look like this

{
"mentor_id": 7,
"org_rep_id": 9,
"start_date": 1614664562,
"end_date": 1619934962,
"notes": "first"
}

After accepting it
when Program sends mentorship request to a mentee(the final request)
the body then looks like this

{
"mentee_id": 8,
"org_rep_id": 9,
"relation_id": 1,
"notes": "second"
}

This is acceptable right? as we are going to give data through BIT

@decon-harsh
Copy link
Author

Let me know If you want to discuss anything or tell me anything, we can talk today in BIT open session 😄

@mtreacy002
Copy link
Owner

mtreacy002 commented Mar 7, 2021

@decon-harsh , I'll try provide feedback whenever I'm available, but you don't need to feel you have to respond to it/work on the requested change immediately. It's ok to take more time on the issue, especially if you have urgent things like exams. So, please focus on that first. Just update me on your status every 3-4 days (if you need more time, e.g. still in exams, or need time to work on it, etc). Just a heads up, on GSoC, this will be a different story as you made the commitment and will be paid for the GSoC tasks, you're accountable to fulfil the tasks as scheduled on the timeline 😉.

@decon-harsh
Copy link
Author

decon-harsh commented Mar 8, 2021

Hey @mtreacy002 I think there is a problem. Take this scenario

  1. Program Sends a program mentorship request to a mentor. A Mentorship_Relation Model instance gets created as mentee field empty. In BIT this relation is extended . Mentorship_Relationship_Extension Model instance is created with program id , mentorship_relation_id , mentor_request_date as datetime.now(). timestamp ()

  2. Mentor accepts the relationship. That Mentorship_Relation Model instance's accept_date became datetime.now(). timestamp(). Same goes for Extension date .

Now, suppose there are two mentees, who want to apply

  1. Mentee 1 sends the program mentorship request to program. That Mentorship_Relation Model instance's mentee field becomes Mentee1.id

  2. It is pending not accepted or rejected yet .

  3. Fairly if it's not accepted or rejected , Mentee 2 can also send the request . But doing so will overwrite the Mentorship_Relation Model fields . Right?

Am I correct ? If so what should be our approach on this!

Moreover i was writing internall calls from BIT and I can assure you user doesn't have to send relation_id field in request body. It will be automatically taken.

@mtreacy002
Copy link
Owner

mtreacy002 commented Mar 9, 2021

Now, suppose there are two mentees, who want to apply

  1. Mentee 1 sends the program mentorship request to program. That Mentorship_Relation Model instance's mentee field becomes Mentee1.id
  2. It is pending not accepted or rejected yet .
  3. Fairly if it's not accepted or rejected , Mentee 2 can also send the request . But doing so will overwrite the Mentorship_Relation Model fields . Right?

Am I correct ? If so what should be our approach on this!

In this situation, we just need to create another MentorshipRelationModel instance on MS backend and a new MentorshipRelationExtentionModel instance on BIT backend (with the same program_id, but different menforship_relation_id) this mentee request. This is because the requests for these 2 mentorship relations are totally separate, the only same thing about them is that they applying to the same program.

However, saying that, I just realised I made an error in the MentorshipRelationExtensionModel 😱 .
In MentorshipRelationModel, I put the program_id as a unique value, but it shouldn't be. Only the mentorship_relation_id should be unique.
This way, the same program can be recorded on multiple rows, but not with the same mentorship_relation_id.
Perhaps it's time we need to write a migration script to fix this. I will open an issue for this on BIT backend for that.

Moreover i was writing internall calls from BIT and I can assure you user doesn't have to send relation_id field in request body. It will be automatically taken.

That's great 👍. It'll be easier to review if we can run the 2 servers (BIT and MS backend and see how they're interacting).

@mtreacy002 mtreacy002 added Category: Coding Changes to code base or refactored code that doesn't fix a bug. Type: Enhancement New feature or request. labels Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Coding Changes to code base or refactored code that doesn't fix a bug. Type: Enhancement New feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants